fix(debuginfo): Remove recursive function parsing - #1063
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4e27604. Configure here.
| for (range, builder) in p.builders.iter_mut() { | ||
| for variable in &p.variables { | ||
| if let Some(variable) = dwarf_unit.variable_for_range(variable, *range) { | ||
| builder.add_variable(variable); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if let Some(line_program) = &dwarf_unit.line_program { | ||
| for (range, builder) in p.builders.iter_mut() { | ||
| for row in line_program.get_rows(range) { | ||
| let address = offset(row.address, dwarf_unit.inner.info.address_offset); | ||
| let size = row.size; | ||
| let file = dwarf_unit.resolve_file(row.file_index).unwrap_or_default(); | ||
| let line = row.line.unwrap_or(0); | ||
| builder.add_leaf_line(address, size, file, line); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (_range, builder) in p.builders { | ||
| output.functions.push(builder.finish()?); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| // Inlinees don't output anything directly, they just contribute to the running list | ||
| // of function builders. | ||
| InProgressSubProgram::Inlined(p) => { | ||
| let Some(builders) = function_stack[p.owning_function_idx].own_builder_mut() else { | ||
| return Err(FunctionBuilderErrorKind::TooManyInlineeNestings.into()); | ||
| }; | ||
| // Create a separate inlinee for each range. | ||
| for range in p.ranges.iter() { | ||
| // Find the builder for the outer function that covers this range. Usually there's only | ||
| // one outer range, so only one builder. | ||
| // | ||
| // We can use `partition_point` here, because builders are sorted by range and | ||
| // non-overlapping, see `parse_ranges`. | ||
|
|
||
| let builder_index = | ||
| builders.partition_point(|(outer_range, _)| outer_range.end <= range.begin); | ||
|
|
||
| let Some((outer_range, builder)) = builders.get_mut(builder_index) else { | ||
| continue; | ||
| }; | ||
| // `partition_point` may return the next builder when `range.begin` falls into a gap between outer ranges. | ||
| if range.begin < outer_range.begin { | ||
| continue; | ||
| } | ||
|
|
||
| let address = offset(range.begin, dwarf_unit.inner.info.address_offset); | ||
| let size = range.end - range.begin; | ||
| let variables = p | ||
| .variables | ||
| .iter() | ||
| .filter_map(|variable| dwarf_unit.variable_for_range(variable, *range)) | ||
| .collect(); | ||
|
|
||
| builder.add_inlinee(FunctionBuilderInlinee { | ||
| depth: p.relative_list_depth as u32, | ||
| name: p.name.clone(), | ||
| address, | ||
| size, | ||
| call_file: p.call_file.clone(), | ||
| call_line: p.call_line, | ||
| variables, | ||
| }); |
There was a problem hiding this comment.
finish() multiplies unbounded ranges × variables with no per-item caps
Regular and Inlined finish paths nest loops over attacker-controlled range lists and variable DIEs (and clone name/call_file per range), so one crafted DWARF unit can force O(R·V) CPU/allocations despite the depth-only max_parse_depth guard.
Evidence
InProgressSubProgram::finish(Regular arm) doesfor (range, builder) in p.buildersthenfor variable in &p.variablescallingvariable_for_range/add_variablewith no R or V cap.- The Inlined arm does the same cross product over
p.rangesandp.variables, plusp.name.clone()/p.call_file.clone()once per range beforeadd_inlinee. - Ranges come from untrusted DWARF range lists via
parse_ranges→range_buf.pushwith no max length; variables are pushed from every nestedDW_TAG_variable/formal_parameterwith no cap. max_parse_depthonly boundsfunction_stack.len()(nesting width of the DIE walk), not range count, variable count, or this finish-time cross product.
Identified by Warden · wrdn-dos-review · L5Z-CPU
loewenheim
left a comment
There was a problem hiding this comment.
I've only reviewed the symcache part in detail so far.
Aside from some nits, I would suggest splitting this up—the symcache and DWARF parts are effectively totally disjoint and don't need to be in the same PR. Also, the change to the max_inline_depth option seems extraneous.
|
|
||
| /// Processes an individual [`Function`], adding its line information to the converter. | ||
| /// | ||
| /// `call_locations` is a non-empty sorted list of `(address, call_location index)` pairs. |
There was a problem hiding this comment.
This line needs to be on InProgressFunction.
| let function = in_progress_function.function; | ||
| let base_idx = in_progress_function.base_index; | ||
| let fn_depth = in_progress_function.depth; | ||
| let call_locations = in_progress_function.call_locations; |
There was a problem hiding this comment.
| let function = in_progress_function.function; | |
| let base_idx = in_progress_function.base_index; | |
| let fn_depth = in_progress_function.depth; | |
| let call_locations = in_progress_function.call_locations; | |
| let InProgressFunction {function, base_index: base_idx, depth: fn_depth, call_locations} = in_progress_function; |
Or deconstruct it in the while let right away.
|
|
||
| let program = self.consume_inline_subprogram_tag( | ||
| next_depth, | ||
| function_stack.len() as isize - (owning_function_idx + 1) as isize, |
There was a problem hiding this comment.
Bug: A potentially negative relative_list_depth is cast from isize to u32 without validation, which can cause silent loss of inlinee information when processing malformed DWARF data.
Severity: MEDIUM
Suggested Fix
Before casting relative_list_depth to u32, add a check to ensure it is not negative. If it is, either return a DwarfError::CorruptedData to signal an error or handle it gracefully to prevent the value from wrapping and causing silent data loss.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: symbolic-debuginfo/src/dwarf/mod.rs#L1307
Potential issue: When parsing DWARF data, the `relative_list_depth` is calculated as an
`isize` which can be negative if the DWARF data is malformed. This `isize` is later cast
to a `u32` without a check for negativity. This causes the negative value to wrap around
to a very large positive integer. Subsequently, a check to see if the depth exceeds
`self.max_function_parse_depth` will pass, causing the inlinee information to be
silently discarded. This results in incomplete stack trace information without raising
any errors.
Also affects:
symbolic-debuginfo/src/dwarf/mod.rs:767~767
loewenheim
left a comment
There was a problem hiding this comment.
Looks good now, aside from two small nits.
| let deadcode_top = function_stack | ||
| .last() | ||
| .is_some_and(|p| matches!(p, InProgressSubProgram::Deadcode(_))); |
There was a problem hiding this comment.
| let deadcode_top = function_stack | |
| .last() | |
| .is_some_and(|p| matches!(p, InProgressSubProgram::Deadcode(_))); | |
| let deadcode_top = matches!( | |
| function_stack.last(), | |
| Some(InProgressSubProgram::Deadcode(_)) | |
| ); |
| let last_func: InProgressSubProgram<'_> = | ||
| function_stack.pop().expect("already checked"); |
There was a problem hiding this comment.
| let last_func: InProgressSubProgram<'_> = | |
| function_stack.pop().expect("already checked"); | |
| let last_func = function_stack.pop().expect("already checked"); |

Rather than futzing with bounds (that change as the code is changed at the stack frame size changes,) just rip the thing out and make an iterative solution for function parsing. This also required de-recursing the writer in symcache, which was much easier (and so closely maps to what we had previously.)
For the dwarf processor, the basic idea is that parse_functions loops around, parsing a single tag each loop, maintaining a stack of in-progress functions. The depth value of the tag drive whether or not to pop elements from the stack (thereby finishing them.) Otherwise, we just push nested elements as we encounter them. There is a nasty edge-case where a function that has invalid/empty ranges can still retain nested functions or inlinees (and the inlinees must be discarded, while the functions must not,) so we guard against when we detect a "dead code" function at the top of the stack, and handle the tags accordingly.